Enabler for rust log to be used in S-CORE#6

Open

pawelrutkaq wants to merge 4 commits intoeclipse-score:mainfrom

qorix-group:pawelrutkaq_initial_log_interface

pawelrutkaq commented Aug 21, 2025 •

edited

Loading

Closes #5

pawelrutkaq force-pushed the pawelrutkaq_initial_log_interface branch from b4f3e69 to c537c0a Compare

August 21, 2025 05:58

pawelrutkaq added 2 commits

August 21, 2025 07:59


          repo: setup according to S-CORE template

a015ea8

Signed-off-by: Pawel Rutka <pawel.rutka.ext@qorix.ai>


          import: import log crate as baseline

0dd7f00

Import from log crate 3aa1359e926a39f841791207d6e57e00da3e68e2
at https://github.com/rust-lang/log

Signed-off-by: Pawel Rutka <pawel.rutka.ext@qorix.ai>

pawelrutkaq force-pushed the pawelrutkaq_initial_log_interface branch from c537c0a to f98cdb1 Compare

August 21, 2025 05:59

pawelrutkaq requested review from armin-acn, arsibo and vinodreddy-g

August 21, 2025 06:03

vinodreddy-g requested review from arsibo and removed request for arsibo

August 21, 2025 06:04

pawelrutkaq requested a review from johannes-esr

August 21, 2025 07:25

pawelrutkaq force-pushed the pawelrutkaq_initial_log_interface branch from f98cdb1 to d718849 Compare

August 22, 2025 06:03


          logging: add subscriber based on baselibs logging

3b3dd62

Signed-off-by: Pawel Rutka <pawel.rutka.ext@qorix.ai>

pawelrutkaq force-pushed the pawelrutkaq_initial_log_interface branch from d718849 to 3b3dd62 Compare

August 22, 2025 10:05

johannes-esr mentioned this pull request

FEO: Integration of Logging Feature eclipse-score/score#1073

Open

darkwisebear self-assigned this

darkwisebear reviewed

View reviewed changes

darkwisebear left a comment

General comment: This looks like a copy & paste from the original log crate. Why not just use that one or re-use / re-export parts? Are there any changes to the crate? If yes, can you please point them out? We can then see how we can integrate those without running into a maintenance nightmare.

src/mw_log_subscriber/src/lib.rs Outdated

+                  }
+                  /// Sets the context for currently build logger.
+                  pub fn context(mut self, context: &str) -> Self {

darkwisebear Sep 12, 2025

Should be called with_context imo.

Author

pawelrutkaq Sep 15, 2025

done

src/mw_log_subscriber/src/lib.rs Outdated

+                  }
+                  /// Returns a slice to filled part of buffer. This is not null-terminated.
+                  fn as_c_str(&self) -> &[c_char] {

darkwisebear Sep 12, 2025

This might be a misnomer: Since it is not null-terminated, it's also not a C string.

Author

pawelrutkaq Sep 15, 2025

yup, changed name

src/mw_log_subscriber/src/lib.rs

+                  /// Returns a slice to filled part of buffer. This is not null-terminated.
+                  fn as_c_str(&self) -> &[c_char] {
+                      unsafe { core::slice::from_raw_parts(self.buf.as_ptr().cast::<c_char>(), self.len()) }

darkwisebear Sep 12, 2025

You should add a SAFETY comment that explains why this is actually safe.

Author

pawelrutkaq Sep 15, 2025

Done

src/mw_log_subscriber/src/lib.rs

+                  }
+                  const fn is_utf8_char_boundary(i: u8) -> bool {
+                      // This is bit magic equivalent to: b < 128 || b >= 192

darkwisebear Sep 12, 2025

I know that this is a copy from the standard lib, but why doing magic if you could just write it like that? The Rust safe coding guidelines will prohibit such casts anyway, so I suggest to turn your comment into code :)

Author

pawelrutkaq Sep 15, 2025

I think it's just 2 compares versus one optimization. Not sure what Rust safe coding has to do here.

darkwisebear Sep 15, 2025

I assume that at some point this library will be used in a safe process. It then needs to comply to the Rust safety guidelines that are being developed. This construct would not be allowed, so sooner or later this has to either be suppressed or be changed.

src/mw_log_subscriber/src/lib.rs

+                  }
+              }
+              struct BufWriter<const BUF_SIZE: usize> {

darkwisebear Sep 12, 2025

Please document what the purpose of this thing is. From reverse engineering the code, it seems you manage a preallocated buffer by copying as many UTF8 bates into the buffer as possibly can fit.

Author

pawelrutkaq Sep 15, 2025

done

src/mw_log_subscriber/src/lib.rs Outdated

+                  log_fn: fn(&mut BufWriter<MSG_SIZE>, &Record),
+              }
+              // SAFETY: The underlying C++ logger is assumed to be safe to change thread

darkwisebear Sep 12, 2025

"assumed" is a bit weak. Turn it into "known" if you know.

Author

pawelrutkaq Sep 15, 2025

done

src/mw_log_subscriber/src/mw_log_ffi.cpp Outdated

+              #include "score/mw/log/logger.h"
+              #include "score/mw/log/log_level.h"
+              namespace score

darkwisebear Sep 12, 2025

Please collapse namespaces here.

Author

pawelrutkaq Sep 15, 2025

done

src/mw_log_subscriber/src/mw_log_ffi.cpp Outdated

+                          extern "C"
+                          {
+                              Logger *mw_log_create_logger(const char *context)

darkwisebear Sep 12, 2025

Is there an equivalent delete?

Author

pawelrutkaq Sep 15, 2025

No as Loggers are maintaned by logging.

darkwisebear Sep 15, 2025

Then it should probably be called "mw_log_get_logger" so that it's clear that this is a non-owning pointer. Maybe you should also elaborate on the lifetime of the pointer.

src/mw_log_subscriber/src/mw_log_ffi.cpp Outdated

+                              void mw_log_fatal_logger(const Logger *logger, const char *message, uint32_t size)
+                              {
+                                  logger->LogFatal() << LogString{message, size};

darkwisebear Sep 12, 2025

Just use std::string_view, should be the cleaner way.

Author

pawelrutkaq Sep 15, 2025 •

edited

Loading

I think this is better as logging says that This class only supports the logging of the following basic data types:: ..., std::string_view (via implicit conversion to mw::log::LogString) so we skip one hop now.

src/mw_log_subscriber/src/mw_log_ffi.rs Outdated


		extern "C" {

		pub(crate) fn mw_log_create_logger(context: const c_char) -> const Logger;

darkwisebear Sep 12, 2025

Nit: The returned value is of type *mut Logger in C++.

Author

pawelrutkaq Sep 15, 2025

Done

Author

pawelrutkaq commented Sep 15, 2025

General comment: This looks like a copy & paste from the original log crate. Why not just use that one or re-use / re-export parts? Are there any changes to the crate? If yes, can you please point them out? We can then see how we can integrate those without running into a maintenance nightmare.

SInce the idea was to break dep to log crate. As there is ongoing rewrite co S-CORE forontend, this is just enabler with clear copy place and location in commit. So there is NO maitenance assumed as this will dissappear and new frontend will only be compatible with log crate.


          logging: Apply fixes requested by review

1a82956

Signed-off-by: Pawel Rutka <pawel.rutka.ext@qorix.ai>

pawelrutkaq requested a review from darkwisebear

September 15, 2025 07:13

darkwisebear commented Sep 15, 2025

General comment: This looks like a copy & paste from the original log crate. Why not just use that one or re-use / re-export parts? Are there any changes to the crate? If yes, can you please point them out? We can then see how we can integrate those without running into a maintenance nightmare.

SInce the idea was to break dep to log crate. As there is ongoing rewrite co S-CORE forontend, this is just enabler with clear copy place and location in commit. So there is NO maitenance assumed as this will dissappear and new frontend will only be compatible with log crate.

It seems that S-CORE wants to deviate from the canonical Rust way of logging, which is the log crate. When doing so, there should be a clear statement which use cases are (and always will be) unsupported by the standard front end. Plus we always should also support the standard crate by implementing a backend that also supports it. Is that the case?

Author

pawelrutkaq commented Sep 15, 2025 •

edited

Loading

General comment: This looks like a copy & paste from the original log crate. Why not just use that one or re-use / re-export parts? Are there any changes to the crate? If yes, can you please point them out? We can then see how we can integrate those without running into a maintenance nightmare.

SInce the idea was to break dep to log crate. As there is ongoing rewrite co S-CORE forontend, this is just enabler with clear copy place and location in commit. So there is NO maitenance assumed as this will dissappear and new frontend will only be compatible with log crate.

It seems that S-CORE wants to deviate from the canonical Rust way of logging, which is the log crate. When doing so, there should be a clear statement which use cases are (and always will be) unsupported by the standard front end. Plus we always should also support the standard crate by implementing a backend that also supports it. Is that the case?

The log crate is not maintaned really in OSSC. Most of things has went into tracing or others. The ideas is that we will provide log crate compatible interface (macros) + S-CORE extensions (ie context passing via macro so DLT can be happy without a need for custom instances ie.). Since log is not certifable, we wont support it like that I think. Thats why it's enabler - otherwise we need to wait longer time for implementation and everone around would use random logging.

Author

pawelrutkaq commented Sep 16, 2025 •

edited

Loading

@arsibo @rmaddikery @hoppe-and-dreams my understanding

mw_log_subscriber can go to logging repo once it's created
mw_log has to go to baselibs
Then we have "clear" start point and can continue with proper impl.

pawelrutkaq mentioned this pull request

import: import log crate as baseline eclipse-score/baselibs#18

Closed

darkwisebear commented Sep 17, 2025

It seems that S-CORE wants to deviate from the canonical Rust way of logging, which is the log crate. When doing so, there should be a clear statement which use cases are (and always will be) unsupported by the standard front end. Plus we always should also support the standard crate by implementing a backend that also supports it. Is that the case?

The log crate is not maintaned really in OSSC. Most of things has went into tracing or others. The ideas is that we will provide log crate compatible interface (macros) + S-CORE extensions (ie context passing via macro so DLT can be happy without a need for custom instances ie.). Since log is not certifable, we wont support it like that I think. Thats why it's enabler - otherwise we need to wait longer time for implementation and everone around would use random logging.

OSSC == Open source scan converter..?

Before deciding to fork the crate (which is effectively what's happening here) we should clarify with upstream whether the necessary changes would be accepted here. Therefore we should collect the known additional parts and open issues upstream. So my questions would be

What do we need in addition to what the log crate offers feature-wise?
What makes us believe that the crate cannot be qualified / what would need to be changed in order to make it qualifiable?

pawelrutkaq mentioned this pull request

Log backend for rust frontend eclipse-score/logging#1

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vinodreddy-g Awaiting requested review from vinodreddy-g

armin-acn Awaiting requested review from armin-acn

arsibo Awaiting requested review from arsibo

johannes-esr Awaiting requested review from johannes-esr

darkwisebear Awaiting requested review from darkwisebear

At least 1 approving review is required to merge this pull request.

Labels

None yet